[TypeSpec APIView] Ensure apiview only triggers on packages directly changed by the pr #31871
Conversation
ckairen
added
the
Central-EngSys
Next Steps to MergeNext steps that must be taken to merge this PR:
|
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| @@ -17,12 +17,6 @@ model Employee is TrackedResource<EmployeeProperties> { | |||
|
|
|||
| /** Employee properties */ | |||
| model EmployeeProperties { | |||
There was a problem hiding this comment.
| model EmployeeProperties { | |
| model EmployeeProperties { | |
| /** Age of employee */ | |
| age?: int32; | |
| /** City of employee */ | |
| city?: string; | |
There was a problem hiding this comment.
temp removal for testing
ckairen
requested review from
weshaggard,
mikeharder,
benbp,
raych1 and
maririos
as code owners
|
Do we really want to show entire npm package tree in log? This makes it difficult to read the logs that's more important. |
You can use the log grouping similar to azure-rest-api-specs/eng/scripts/Create-APIView.ps1 Lines 143 to 147 in d7d8b18 which will hide the lengthy logs in a toggle. That way people can still expand to view it if they want to. |
eng/scripts/Create-APIView.ps1
Outdated
| while ($filePathParts.Length -and !$configFilesInTypeSpecProjects) { | ||
| $filePathParts = $filePathParts | Select-Object -SkipLast 1 | ||
| $typeSpecProjectBaseDirectory = $filePathParts -join [IO.Path]::DirectorySeparatorChar | ||
| $configFilesInTypeSpecProjects = Get-ChildItem -Path $typeSpecProjectBaseDirectory -File "tspconfig.yaml" |
There was a problem hiding this comment.
This will still be an issue. It is possible to have tspconfig.yaml in sub folders for a shared project. Current logic will stop looking further upwards and move to next block to check if current directory has main.tsp. It won't come back and check again the parent path of current file path if there is no main.tsp.
We can simplify this logic that will work for shared projects also.
for each changed files, strip the prefix before `specification/' from path:
- check if current path has tspconfig.yaml and main.tsp.
- If yes, add this folder to project set
- If no, then go upwards and continue step 2 until we match current path to 'specification/[^\/]+/+`
APIView needs to be generated from paths that contain both main.tsp and tspconfig.yaml (at least to retain same behavior as current one.
There was a problem hiding this comment.
Hmm I don't think I understand the exact situation where this might not work.
The code right now is checking all parent directory of the changed file till we reach root.
line 96 $filePathParts = $filePathParts | Select-Object -SkipLast 1 deletes the leaf directory on each iteration
There was a problem hiding this comment.
update: now using split-path -parent but the iterating logic still remains the same
microsoft-github-policy-service
bot
added
no-recent-activity
|
I disabled the TypeSpec APIView pipeline until this is merged. From my testing I think this PR resolves the TypeSpec project discovery bug but giving there are many ongoing discussions here I will let @ckairen finish it up. For now, the legacy TypeSpec APIView is still running and will generate APIView. This pipeline is causing some confusion at the moment. Please enable re-enable https://dev.azure.com/azure-sdk/public/_build?definitionId=7325 when this is merged. |
specification/confidentialledger/Microsoft.ManagedCcf/transactions.tsp
Outdated
Show resolved
Hide resolved
| @@ -31,9 +31,6 @@ model WidgetSuite { | |||
| @visibility("read") | |||
| name: string; | |||
|
|
|||
| @doc("The ID of the widget's manufacturer.") | |||
| manufacturerId: string; | |||
|
|
|||
| @doc("The faked shared model.") | |||
There was a problem hiding this comment.
| @doc("The faked shared model.") | |
| @doc("The ID of the widget's manufacturer.") | |
| manufacturerId: string; | |
| @doc("The faked shared model.") |
There was a problem hiding this comment.
temp removal for testing 2 directories
closes #31870